feat(s3-deployment): apply least privilege to destination bucket policies #35663
Conversation
github-actions
bot
added
effort/medium
github-actions
bot
added
the
beginning-contributor
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
amandladev
force-pushed
the
feature/s3-deployment-permissions-least-privilege
branch
from
a4fd28a to
f17cf5b
Compare
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
5 similar comments
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
aws-cdk-automation
added
the
pr/needs-further-review
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
3 similar comments
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
There was a problem hiding this comment.
Hi @amandladev ,
Thanks for your contribution. I have left a few comments.
| * @param prefix prefix string | ||
| * @returns normalized prefix string | ||
| */ | ||
| normalizePrefix(prefix?: string): string { |
There was a problem hiding this comment.
we can make this a private method.
| ? `${this.normalizePrefix(props.destinationKeyPrefix)}*` | ||
| : '*'; | ||
|
|
||
| this.destinationBucket.grantReadWrite(handler, objectPattern); |
There was a problem hiding this comment.
By default prune is true, which means when we remove source files from the CDK app, the lambda tries to clean up the corresponding files in S3. But it fails because it doesn't have delete permissions for paths that are no longer in the app.
I tested this by:
- deploying a cdk app with stuff in
app/frontendandapp/backend - removing
app/backendsource and deploying it again - second deployment fails because lambda can't delete the old backend files
We should give the lambda delete permissions for the whole bucket (*) to make deletion work properly.
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
1 similar comment
|
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
3 participants
Introduction
This change improves security by scoping IAM policies to the specific
destination key prefix instead of granting access to all bucket objects.
Changes:
/<destinationKeyPrefix>/*instead of/*destinationKeyPrefixis specified (e.g., 'deploy/here/', 'efs/'),the Lambda execution role only receives permissions for that specific prefix
/*accessSecurity Benefits:
Affected Use Cases:
✅ Deployment with prefix:
destinationKeyPrefix: 'deploy/here/'✅ EFS-backed deployment:
destinationKeyPrefix: 'efs/', useEfs: true✅ Multiple deployments to same bucket with different prefixes
✅ Deployments without prefix (unchanged behavior)
Testing:
Fixes #35610